adding required methods and functions for "__typename must be const" change #137
Conversation
✅ Deploy Preview for grats ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Nice work, this is looking great! Next steps would be:
Tips on running fixtures tests can be found here: https://github.com/captbaritone/grats/blob/main/CONTRIBUTING.md#automated-tests |
There was a problem hiding this comment.
Looking good! A few comments about error messages.
Also, this would be a great candidate for a code action. I landed support for them yesterday: 0b53e3d
If you'd like to add support for that as part of this change, you can see an example here. (you'll need to rebase)
Also happy to add this myself or have that be a followup PR.
Finally, can search through the website directory for instances of __typename and see if there are any changes that need to be made there?
| return false; | ||
| } | ||
|
|
||
| if (node.initializer.type.typeName.escapedText !== "const") { |
There was a problem hiding this comment.
| if (node.initializer.type.typeName.escapedText !== "const") { | |
| if (node.initializer.type.typeName.escapedText !== ts.SyntaxKind.ConstKeyword) { |
There was a problem hiding this comment.
I've tried this thing , but getting this error
and the reason is
There was a problem hiding this comment.
Interesting. I guess it doesn't parse as const as a keyword! Leaving this as a string is fine.
src/Errors.ts
Outdated
| } | ||
|
|
||
| export function typeNameTypeNotReferenceNode() { | ||
| return `Expected \`__typename\` property type to be a TypeReferenceNode. ${TYPENAME_CONTEXT}`; |
There was a problem hiding this comment.
I'd love to try to avoid using AST type names in error messages if possible. I don't think most developers know (or should need to know) what a TypeReferenceNode or Identifier are. Maybe we should just simplify this down to one error message that looks more like the one in typeNameInitializeNotExpression and use it for all of these new error cases?
|
Also, can we make sure to have a fixture test for every error case? I don't think I see some of the new error messages represented in the test fixtures. |
…t-be-const---fix
|
@captbaritone . sir can you please review it and let me know for the required changes .. |
|
Thanks! I had some commits on main which I had to revert (unrelated to this PR) which meant I had to manually merge as 1eed523 You can see your commits (plus a few additions/and fixups) there. |
Resolves #122
This pr will add
Before -
After
Fix
@captbaritone Please review , and let me know for the change's